feat(trajectory): adjudicate redaction findings per item in bug reports - #381
Conversation
gloryfromca
left a comment
gloryfromca
left a comment
There was a problem hiding this comment.
Blocking: stop the completeness probe from leaking SkillFileWatcher threads and crashing at process shutdown.
I reviewed the full github/main...HEAD diff and the surrounding bundle, replay, provider, pricing, store, CLI, and watcher call paths. I also checked the branch history, backward-compatible record validation, test changes for weakening, and the repository rules in AGENTS.md/CLAUDE.md plus the canonical trajectory terms in CONTEXT-MAP.md and CONTEXT.md. The per-item decision, redaction, path-sanitization, frozen-package, retry, and CLI authorization flows otherwise look internally consistent.
Verification: uv run pytest tests/test_trajectory_redact.py tests/test_trajectory_review.py tests/test_trajectory_bugreport.py tests/test_trajectory_completeness.py tests/test_cli_trajectory_commands.py tests/test_cli_trajectory_browse.py tests/test_provider_rates.py tests/test_token_wise_pricing.py -x reached 499 passed in 16.38s, then segfaulted and exited 139. The isolated completeness reproducer also prints 1 passed and exits 139; preventing ContextBuilder from starting its watcher makes that same two-replay reproducer exit 0.
| evidence damage. | ||
| """ | ||
| try: | ||
| report = asyncio.run(run_replay(tree, mode="warn")) |
There was a problem hiding this comment.
Blocking: each completeness probe leaks a live SkillFileWatcher, causing repeated probes and the changed test suite to segfault at process shutdown.
run_replay constructs an AgentLoop, whose default ContextBuilder starts the Rust-backed watcher, but neither replay nor this caller stops it. This is reproducible with uv run pytest tests/test_trajectory_completeness.py::test_complete_sample_probes_clean -q: it prints 1 passed but exits 139; after its two replay calls, threading.enumerate() shows two live SkillFileWatcher threads. Overriding ContextBuilder with start_watcher=False makes the same reproducer exit 0. The interactive trajectory browser can invoke this probe repeatedly, so the probe needs to avoid starting the watcher or tear the loop's watcher down.
There was a problem hiding this comment.
Fixed in bf934b0. The stop lives at the run_replay level rather than only in the probe: right after the AgentLoop is built, its ContextBuilder's skill watcher is stopped via the catalog's stop_file_watcher() (a replay feeds recorded content only, so the watcher has nothing to do), which covers the completeness probe, the replay CLI, and the regression runner in one place. The end-to-end recording tests in test_trajectory_replay.py stop the watcher on their five directly-built loops too (their leak pre-existed on the base branch), and a new regression test asserts run_replay leaves no SkillFileWatcher threads alive. Verification: your isolated reproducer (test_complete_sample_probes_clean) now exits 0, the full 8-file command reaches 499 passed with exit 0, and test_trajectory_replay.py alone is 32 passed exit 0 (previously 139).
There was a problem hiding this comment.
Blocking: remove the transient PR-review provenance from the new test docstring before this revision merges.
The watcher leak itself is resolved by bf934b0. Stopping the watcher in run_replay covers the completeness probe and the other replay callers, the original isolated reproducer now exits 0, and the full changed-area run completes normally with 531 passing tests. The remaining blocker is separate from this thread and is marked on the new test docstring.
gloryfromca
left a comment
gloryfromca
left a comment
There was a problem hiding this comment.
Blocking: remove the transient PR-review provenance from the new regression-test docstring.
The watcher lifecycle fix is correct at the shared run_replay layer: I checked the new delta, replay/completeness/browser callers, watcher stop semantics, branch history, backward compatibility, test changes for weakening, and the repository rules in AGENTS.md/CLAUDE.md plus the trajectory terminology in CONTEXT-MAP.md and CONTEXT.md. I found no other issue in this revision.
Verification: the original reproducer now completes with 1 passed and exit 0. uv run pytest tests/test_trajectory_redact.py tests/test_trajectory_review.py tests/test_trajectory_bugreport.py tests/test_trajectory_completeness.py tests/test_trajectory_replay.py tests/test_cli_trajectory_commands.py tests/test_cli_trajectory_browse.py tests/test_provider_rates.py tests/test_token_wise_pricing.py -x completed with 531 passed in 21.03s and exit 0.
| async def test_run_replay_leaves_no_skill_watcher_threads(tmp_path) -> None: | ||
| """The loop's ContextBuilder starts a skill file watcher; a replay must | ||
| stop it, or repeated probes leak daemon threads that crash the process at | ||
| interpreter shutdown (found by PR review on the completeness probe).""" |
There was a problem hiding this comment.
Blocking: remove the transient PR-review provenance from this test docstring.
The parenthetical found by PR review is historical task provenance, not part of the behavior being specified. AGENTS.md section 1.1 explicitly forbids repository comments that reference a PR or describe transient task context because they become stale after the task. The rest of the docstring already explains the regression completely, so please drop only that parenthetical.
There was a problem hiding this comment.
Fixed in e363812: the parenthetical is gone, and the same cleanup was applied to three sibling docstrings that carried the same kind of transient wording (review-verified trap / reviewed-verified leak in test_trajectory_review.py, test_trajectory_bugreport.py, test_cli_trajectory_browse.py) - the behavior descriptions stand on their own now. The same commit also fixes the CI failure this test caused under the full-suite process: the assertion was global, counting watcher threads leaked by other suites in the same process; it now snapshots SkillFileWatcher threads before the replay and asserts the replay added none. Verified with a leaking suite run first in the same process (tests/test_decision_consumer.py + tests/test_trajectory_replay.py -> 41 passed, exit 0) and the replay file alone (32 passed, exit 0).
There was a problem hiding this comment.
No blockers; this can merge as far as I am concerned.
Confirmed: the transient provenance wording is gone, and scoping the watcher assertion to thread identities created during this replay preserves the regression check without depending on unrelated suite-owned watchers. The 531 changed-area tests pass with exit 0.
gloryfromca
left a comment
gloryfromca
left a comment
There was a problem hiding this comment.
No blockers; this can merge as far as I am concerned.
Both prior findings are resolved. I reviewed the new delta and rechecked its replay/completeness/browser callers, watcher lifecycle semantics, branch history, backward compatibility, test changes for weakening, and the repository rules in AGENTS.md/CLAUDE.md plus the trajectory terms in CONTEXT-MAP.md and CONTEXT.md. The delta-scoped watcher assertion remains capable of detecting a thread introduced by run_replay, while tolerating pre-existing watchers owned by other suites.
Verification: the four directly touched test files completed with 290 passed in 10.80s. The full changed-area command completed with 531 passed in 17.45s; both exited 0. A full uv run pytest -x run stopped after 1750 passed on tests/test_cli_theme.py::test_bold_accent_renders_styled_not_bare; that test also fails in isolation, and neither it nor its implementation is changed by this branch, so I did not attribute it to this revision.
Residual findings now carry the plaintext token and every occurrence (file, line number, line text, offsets) in memory, so the upcoming review flow can adjudicate by value and render highlighted context. redaction.json keeps its exact serialized shape - neither the token nor the raw lines land on disk. Provider tool-call ids (call_ prefix) join the residual-scan exemptions alongside tracing ids. Co-authored-by: Claude (claude-fable-5) <noreply@anthropic.com>
New review module: build_review_items merges residual findings by token value across every redaction report (one decision per value, since replacement is global by value), names sources in human terms via the bundle's span records, and derives the confirmed private-key item from the replacement placeholders. validate_review_decisions is side-effect free so interactive callers re-ask on linked-value conflicts before anything is applied. apply_review_decisions counts kept tokens over the body files before any change, replaces user-redacted values through the existing exact-replacement engine, prunes them from the residual findings before rewriting each tree's redaction.json, filters every string headed for serialization, and verifies the result: redacted variants gone everywhere, kept counts unchanged. Co-authored-by: Claude (claude-fable-5) <noreply@anthropic.com>
Fold the blocked classification into the review flow: a private-key hit becomes a review reason plus a confirmed-sensitive item to acknowledge (the content is already replaced), no longer a hard stop. freeze_export gains a decide callback invoked when review items exist - decisions are validated conflict-free before anything is applied, the problem fields are read only after the decisions rewrote the redacted trees, the completeness probe runs on the post-decision tree, and the export gate also scans the package metadata for user-redacted spellings. Records and packages carry the new additive redaction fields security_notices and user_decisions (absent on old records, validated when present); a report whose findings were all replaced stays needs_review so risk_accepted remains truthful. The browser and the CLI TTY path share one review screen: full item list with semantic sources and highlighted context, keep/replace per suspected item, acknowledge for confirmed ones, conflict re-asking scoped to the linked group, and a single risk-worded confirmation that --yes never skips. Without a TTY, --accept-risk acknowledges confirmed items and keeps all findings (recorded as such); without it the command fails and lists the reasons, leaving nothing behind. Co-authored-by: Claude (claude-fable-5) <noreply@anthropic.com>
Co-authored-by: Claude (claude-fable-5) <noreply@anthropic.com>
Three fixes from the round-1 code review: - A redacted value could still ship as a tar member name: artifact files are named after payload-derived basenames, and only contents were rewritten. apply_review_decisions now renames export paths with the exact replacement the content got (so the rewritten references keep resolving), aborts on rename collisions, filters kept findings' file fields and skipped-binary names before the summaries are rewritten, and verifies member paths alongside contents. - Esc/Ctrl-C during CLI adjudication escaped as browser-internal exceptions; the CLI decide wrapper converts both to the review cancel, printing the cancel line and leaving nothing behind. - Suspected items effectively defaulted to Keep (the pointer starts on the first select row): the row is now a placeholder that records nothing and re-asks, so a bare Enter cannot silently keep an unknown token; the private-key acknowledge default stays as designed. Co-authored-by: Claude (claude-fable-5) <noreply@anthropic.com>
Co-authored-by: Claude (claude-fable-5) <noreply@anthropic.com>
report-bug grows --keep-findings / --redact-findings (mutually exclusive, checked before any collection side effect): each flag pre-decides every suspected item, --accept-risk acknowledges the confirmed-sensitive ones and keeps its role as the review-report authorization. Without a TTY, missing flags fail at the decision point against the original items - before any replacement or freeze work - printing every item with its semantic sources and highlighted context plus all missing flags at once, and leaving nothing behind. On a TTY the flags skip the prompts for their kind while the risk consent still gates a run without --accept-risk; --yes keeps meaning only the plain creation confirmation. Co-authored-by: Claude (claude-fable-5) <noreply@anthropic.com>
Provider usage blocks carry fixed field names (completion_tokens_details, prompt_tokens_details) that clear the entropy bar in every trajectory with a model call; they join the residual-scan exemptions as exact literals - any variation still flags. User decisions gain a masked_token field (first/last four characters of the adjudicated value; the placeholder for confirmed items) so screens can show a short identifier instead of the whole sample window; the field is optional on load, records written before it exist stay valid. Co-authored-by: Claude (claude-fable-5) <noreply@anthropic.com>
…ions Review items now lead with the plaintext token and its occurrence count, group identical context lines (every hit on a short line stays highlighted and counted), render whole lines up to 300 characters with a 200-character window beyond that, and list every location as "semantic source - file:line" under each context. Only context text is capped at five groups: the remaining occurrences still list all their locations, marked context omitted, and the screen header states what the paths are relative to. Suspected decisions are now single-letter input (k/r/c; empty or unknown input re-asks - no default), replacing the placeholder select row; the confirmed item keeps Enter-equals- acknowledge. The confirmation summary shows a Reviewed tally (kept / redacted / acknowledged) with short masked tokens and aggregated sources instead of raw sample windows, and a completed review adds a plain-language line before the risk consent. Authorization semantics are unchanged. Co-authored-by: Claude (claude-fable-5) <noreply@anthropic.com>
Two hits on a line just over the whole-line limit can render the same windowed text from different positions; the context group now collects every occurrence's spans (deduplicated) and the renderer merges overlapping ranges before painting, so no hit loses its highlight to display-text grouping. Co-authored-by: Claude (claude-fable-5) <noreply@anthropic.com>
run_replay builds an AgentLoop whose ContextBuilder starts the Rust-backed skill file watcher, and nothing ever stopped it: every completeness probe leaked a daemon thread and repeated probes (or the test suite) crashed the process at interpreter shutdown (exit 139). A replay feeds recorded content only, so the watcher is stopped right after the loop is built - covering the completeness probe, the replay CLI, and the regression runner. The end-to-end recording tests stop the watcher on their directly-built loops too, and a regression test asserts a replay leaves no watcher threads behind. Co-authored-by: Claude (claude-fable-5) <noreply@anthropic.com>
The watcher regression test asserted zero SkillFileWatcher threads globally, which fails under the full-suite CI process where other suites hold their own watcher threads: it now snapshots the threads before the replay and asserts the replay added none. Test docstrings also drop the transient review-provenance wording; the behavior descriptions stand on their own. Co-authored-by: Claude (claude-fable-5) <noreply@anthropic.com>
e363812 to
3997f5a
Compare
gloryfromca
left a comment
gloryfromca
left a comment
There was a problem hiding this comment.
No blockers; this can merge as far as I am concerned.
This is a history-only refresh: git diff e363812a253c HEAD is empty, so the reviewed source and test tree is unchanged. I refreshed the target ref and rechecked the actual github/main...HEAD diff and rewritten history against the repository rules; caller behavior, backward compatibility, architectural fit, and test strength remain as previously reviewed.
Verification: uv run pytest tests/test_trajectory_completeness.py::test_complete_sample_probes_clean tests/test_trajectory_replay.py::test_run_replay_leaves_no_skill_watcher_threads -q completed with 2 passed.
4 participants
Summary
Bug reports previously classified redaction into clean / needs_review /
blocked, where a private-key hit refused to produce a report and the
needs_review confirmation showed at most 5 masked samples pointing at a
file buried inside the frozen snapshot. This PR folds blocked into a
per-item review flow:
keep as-is, or replace with [REDACTED:user-confirmed] (all four
spelling variants replaced and verified gone, tar member names
included - files carrying a redacted value are renamed with the same
replacement their references got). Each item leads with the token and
its occurrence count, groups identical context lines (every hit
highlighted), and lists every location as semantic source plus
file:line ("model call feat(security): label untrusted content and contain injection blast radius #2 input - artifacts/...json:57"); decisions
are single-letter input (k/r/c) with no default. The user never has
to open a file, and can still locate everything.
replaced, the reporter acknowledges the risk per item, and the package
carries an explicit security notice.
double prompt; --yes never implies the risk authorization. The
summary shows a Reviewed tally (kept / redacted / acknowledged) with
short masked tokens, keeping completed decisions apart from the
warnings that still need consent.
and user_decisions (with a masked_token short identifier; absent on
old records, validated when present); the persisted classification
enum is unchanged, so old and new readers stay compatible both ways.
suspected items per kind for scripts; a run missing flags fails before
any content change, listing every item and every missing flag at once.
names (completion_tokens_details, prompt_tokens_details; exact
literals only) join the residual-scan exemptions.
recovery), the completeness probe, path sanitization, tar header
anonymization, the expert command raven trajectory report, and the
record state machine.
Note: this branch is stacked on feat/trajectory_bug_report (#380); the
diff includes those commits until #380 merges, after which this branch
will be rebased onto main.
Type
Verification
uv run pytest tests/test_trajectory_redact.py tests/test_trajectory_review.py tests/test_trajectory_bugreport.py tests/test_cli_trajectory_browse.py tests/test_cli_trajectory_commands.py -q-> 380 passeduv run pytest tests/ --ignore=tests/integration-> 7156 passed (the 11 failures / 19 errors are pre-existing environment issues on this machine - missing tzdata and similar - identical on the base branch)uv run --frozen --extra dev ruff check raven/ tests/-> passeduv run --frozen --extra dev ruff format --check raven/ tests/-> 861 files already formattedmake check-large-files-> passedRelevant tests pass locally
Relevant lint / type checks pass locally
User-facing docs or screenshots are updated when needed
Risk
User-visible changes: the blocked refusal wall is gone (replaced by an
informed acknowledgment with an in-package notice), the review
confirmation is a single risk-worded prompt instead of two, and
non-interactive review runs now require an explicit --keep-findings or
--redact-findings for suspected items (previously --accept-risk alone
shipped them as kept). Rollback: revert the squash commit; the new
metadata fields are additive, so already-produced packages and records
keep loading either way.
Related Issues
N/A